Skip to content

Add retry page for locations and location details page - #619

Open
shakilhossain1 wants to merge 2 commits into
mainfrom
feature/add-retry-page
Open

Add retry page for locations and location details page#619
shakilhossain1 wants to merge 2 commits into
mainfrom
feature/add-retry-page

Conversation

@shakilhossain1

Copy link
Copy Markdown
Collaborator

No description provided.

@jbeard4
jbeard4 temporarily deployed to yourpeer-feature-add-re-8icl3h March 10, 2026 10:38 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR introduces broad error handling, but it appears to break Next.js redirect flow and masks unexpected server failures, which is not safe to approve yet.

Blocking issues

  • redirectIfNearbyAndIfLatitudeAndLongitudeIsNotSet(...) is now wrapped in try/catch and non-Error404Response errors are swallowed in fallback UI paths. In Next.js, redirect() works by throwing; these catches will intercept redirect exceptions and prevent intended redirects. Affected files: src/app/[route]/@mapContainer/page.tsx, src/app/[route]/@sidePanel/page.tsx, and the two [locationSlugOrPersonalCareSubCategory]/page.tsx variants.
  • Unexpected non-404 errors are no longer rethrown and now return empty map / side-panel error UI (200 render) instead of surfacing 500. This can hide real backend failures and cause silent behavioral regressions (stale/empty content with no error boundary).
  • Risky control-flow changes (redirect/error semantics) were added without accompanying tests to verify redirect behavior, 404 handling, and non-404 failure paths; this leaves high regression risk unguarded.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 153f0d890d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +47 to +51
if (e instanceof Error404Response) {
return notFound();
}

return <LocationsMap locationStubs={[]} />;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-throw redirect control-flow errors in catch block

redirectIfNearbyAndIfLatitudeAndLongitudeIsNotSet calls Next.js redirect(...), which signals redirects by throwing a control-flow error; this catch now treats any non-Error404Response as a recoverable failure and renders an empty map instead. In the sortBy=nearby + missing lat/long cookie flow, users will no longer be redirected to the non-nearby URL and will land on the error/empty state, which is a regression in navigation behavior.

Useful? React with 👍 / 👎.

@jbeard4
jbeard4 temporarily deployed to yourpeer-feature-add-re-8icl3h March 10, 2026 10:52 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR introduces broad error swallowing in route pages, which can mask real server failures as successful empty states. Given the behavioral change and lack of accompanying tests for these fallback paths, this is not safe to approve conservatively.

Blocking issues

  • Unexpected non-404 errors are now converted into successful UI fallbacks (<LocationsMap locationStubs={[]}> / <SidePanelErrorState />) instead of propagating as 500s in multiple pages. This can hide backend outages and silently change failure semantics for users and monitoring.
  • Returning an empty map on server/data failures is behaviorally ambiguous and can be interpreted as 'no locations exist' rather than 'load failed', which is a potential regression in core user flow.
  • No tests are included for the new error-handling branches (404 vs non-404, detail vs list routes, retry state behavior). For risky control-flow changes in data-loading pages, this missing coverage is blocking.

@jbeard4
jbeard4 temporarily deployed to yourpeer-feature-add-re-xqpt17 March 11, 2026 12:23 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR introduces broad non-404 error swallowing and fallback rendering paths without test coverage, which is risky and can hide real production failures behind 200 responses.

Blocking issues

  • src/app/[route]/@mapContainer/[locationSlugOrPersonalCareSubCategory]/page.tsx and src/app/[route]/@sidePanel/[locationSlugOrPersonalCareSubCategory]/page.tsx: non-404 exceptions that previously surfaced as 500s are now converted into fallback UI (LocationsMap with empty stubs or SidePanelErrorState). This masks backend/server faults, changes failure semantics, and can break monitoring/alerting expectations.
  • src/app/[route]/@mapContainer/page.tsx and src/app/[route]/@sidePanel/page.tsx: broad catch blocks return successful fallback UI for any non-404 error, creating potential silent regressions instead of explicit failures.
  • No tests are included for the new error-handling behavior (non-404 failure paths, retry state behavior, and fallback rendering), leaving risky logic changes unverified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants